Skip to content

[Router] Require classifier runtime weights so partial downloads heal - #2820

Open
olifarhaan wants to merge 2 commits into
vllm-project:mainfrom
olifarhaan:fix/classifier-model-completeness
Open

[Router] Require classifier runtime weights so partial downloads heal#2820
olifarhaan wants to merge 2 commits into
vllm-project:mainfrom
olifarhaan:fix/classifier-model-completeness

Conversation

@olifarhaan

@olifarhaan olifarhaan commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Closes #2669

Purpose

On the mmBERT-32K backend, which is the default for all three, the category, PII, and jailbreak classifiers load through TraditionalModernBertTokenClassifier::new_with_variant. It hard-reads config.json, tokenizer.json, and model.safetensors from the model root and fails outright if any is missing

Completeness only demanded config.json plus any weight-shaped file, and hasModelWeights matches *.safetensors, *.bin, and *.onnx recursively. So an interrupted download that left the companion mapping and a nested adapter blob behind read as complete, was never re-fetched, and since these classifiers register with bestEffort: false, every restart failed the same way

addClassifierModelRequiredFiles mirrors the existing addEmbeddingModelRequiredFiles from #2172 and records the root weights and tokenizer for the three models. Module affected: Router

On the LoRA caveat raised in the issue: this is gated on use_mmbert_32k, which is exactly the flag selecting the initialiser that has no LoRA fallback. With it off, PII and jailbreak init auto-detects LoRA directories that legitimately carry adapter weights instead of root weights, so those keep the looser heuristic. A test pins that carve-out

Test Plan

go test ./pkg/modeldownload/ -count=1
make test-semantic-router
make go-lint
make check-go-mod-tidy
make agent-ci-lint CHANGED_FILES="src/semantic-router/pkg/modeldownload/config_parser.go,src/semantic-router/pkg/modeldownload/classifier_completeness_test.go"

To confirm the tests are not vacuous, revert each part of the fix and re-run

Test Result

All four new tests fail on the base for the right reason, for example:

--- FAIL: TestBuildModelSpecsRequiresClassifierRuntimeWeights
    models/mmbert32k-pii-detector-merged RequiredFiles = []string{"config.json", "pii_type_mapping.json"}, missing "model.safetensors"
--- FAIL: TestPartialClassifierDirReportedIncomplete
    partial classifier dir reported complete; the runtime hard-loads model.safetensors and would fail at init

Reverting each part of the change in turn fails at least one test every time:

Reverted Fails
addClassifierModelRequiredFiles call dropped TestBuildModelSpecsRequiresClassifierRuntimeWeights, TestPartialClassifierDirReportedIncomplete
use_mmbert_32k gate dropped, so LoRA also demands root weights TestLoRAClassifierKeepsHeuristicCompleteness

make go-lint reports 0 issues, make check-go-mod-tidy and make agent-ci-lint pass, and every applicable pre-commit hook passes. make test-semantic-router reports one failure, TestHybridCachePendingRequest in pkg/cache, which is a known flake unrelated to this change. It fails on other contributors' PRs that do not touch pkg/cache either (#2530, #2507, #2810), and it passed on my own PR #2819. The test writes to the Milvus-backed hybrid cache, sleeps a fixed 100ms "for indexing", then asserts the entry is findable, so a loaded runner fails it. Locally it cannot run at all without a Milvus instance, which is why make test-semantic-router defaults SKIP_MILVUS_TESTS=true. It is currently failing on several unrelated PRs and on main itself

One thing I did not change: DefaultRequiredFiles still allows the loose nested-weight heuristic for every other model. Tightening that globally would be a much wider change than this issue

The models/ prefix is now a named constant, since the linter flags the third repetition this change would have introduced

@netlify

netlify Bot commented Aug 8, 2026

Copy link
Copy Markdown

Deploy Preview for vllm-semantic-router ready!

Name Link
🔨 Latest commit 75387de
🔍 Latest deploy log https://app.netlify.com/projects/vllm-semantic-router/deploys/6a8b0ca390801800086d848f
😎 Deploy Preview https://deploy-preview-2820--vllm-semantic-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

👥 vLLM Semantic Team Notification

The following members have been identified for the changed files in this PR and have been automatically assigned when their GitHub accounts are assignable in this repository:

📁 src/semantic-router

Owners: @FAUST-BENCHOU, @shraderdm, @drivebyer, @ramkrishs, @WUKUNTAI-0211, @AayushSaini101, @siloteemu
Files changed:

  • src/semantic-router/pkg/modeldownload/classifier_completeness_test.go
  • src/semantic-router/pkg/modeldownload/config_parser.go

vLLM Semantic Router

🎉 Thanks for your contributions!

This comment was automatically generated based on the OWNER files in the repository.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

✅ Supply Chain Security Report — All Clear

Scanner Status Findings
AST Codebase Scan (Py, Go, JS/TS, Rust) 31 finding(s) — MEDIUM: 24 · LOW: 7
AST PR Diff Scan No issues detected
Regex Fallback Scan No issues detected

Scanned at 2026-08-08T16:46:44.786Z · View full workflow logs

@Xunzhuo Xunzhuo added the pr/blocked Blocked on a named decision, dependency, or required check. label Aug 22, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 33.93%. Comparing base (3434d6b) to head (0ef8833).
⚠️ Report is 76 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2820      +/-   ##
==========================================
+ Coverage   33.83%   33.93%   +0.09%     
==========================================
  Files          20       20              
  Lines        2935     2959      +24     
==========================================
+ Hits          993     1004      +11     
- Misses       1840     1849       +9     
- Partials      102      106       +4     
Flag Coverage Δ
operator 33.93% <ø> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

On the mmBERT-32K backend the category, PII, and jailbreak classifiers load
through TraditionalModernBertTokenClassifier, which hard-reads config.json,
tokenizer.json, and model.safetensors from the model root. Completeness only
demanded config.json plus any weight-shaped file anywhere in the tree, so an
interrupted download that left a companion mapping and a nested adapter blob
behind read as complete and was never re-fetched, while init kept failing.

Mirror addEmbeddingModelRequiredFiles and record the root weights and tokenizer
for these three models. Restrict it to the mmBERT-32K path, which initialises
with no fallback; the candle variant auto-detects LoRA directories that
legitimately carry adapter weights instead, and requiring root weights there
would strand a valid model in a permanent re-download loop. A prompt guard with
a Protocol set runs remotely and has no local model at all.

Signed-off-by: Mohammad Ali Farhan <alifarhan231087@gmail.com>
Pin the required-file contract for all three classifiers, the partial-download
directory that used to read as complete, the fully downloaded control, and the
candle backend that must keep the looser heuristic.

Signed-off-by: Mohammad Ali Farhan <alifarhan231087@gmail.com>
@olifarhaan
olifarhaan force-pushed the fix/classifier-model-completeness branch from 0ef8833 to 75387de Compare August 23, 2026 15:07
@Xunzhuo Xunzhuo added the wg/router-models-inference-runtime Owned by the Router Models and Inference Runtime Workgroup. label Aug 28, 2026
@github-actions github-actions Bot added pr/needs-rebase Needs rebase or conflict resolution. wg/router-models-inference-runtime Owned by the Router Models and Inference Runtime Workgroup. pr/needs-review Ready for reviewer attention. and removed wg/router-models-inference-runtime Owned by the Router Models and Inference Runtime Workgroup. pr/blocked Blocked on a named decision, dependency, or required check. pr/needs-rebase Needs rebase or conflict resolution. labels Aug 28, 2026
@github-actions github-actions Bot added pr/needs-rebase Needs rebase or conflict resolution. and removed pr/needs-review Ready for reviewer attention. labels Sep 1, 2026
@github-actions github-actions Bot added pr/needs-review Ready for reviewer attention. and removed pr/needs-rebase Needs rebase or conflict resolution. labels Sep 2, 2026
@Xunzhuo

Xunzhuo commented Sep 2, 2026

Copy link
Copy Markdown
Member

Thanks for adding the classifier completeness regressions. This branch currently conflicts with main, so please rebase it onto current main and rerun the model-download tests; I will review the refreshed head.

@github-actions github-actions Bot added pr/needs-rebase Needs rebase or conflict resolution. pr/needs-review Ready for reviewer attention. and removed pr/needs-review Ready for reviewer attention. pr/needs-rebase Needs rebase or conflict resolution. labels Sep 2, 2026

@Xunzhuo Xunzhuo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provisioning discovers request-reachable named recipes, but required classifier files are added only from flat/default fields. A named reachable mmBERT recipe can therefore download without mandatory root weights/tokenizer and be accepted as complete. Please apply completeness requirements to the same reachable profiles and cover a named entrypoint.

@github-actions github-actions Bot added pr/needs-author Waiting for author changes or response. and removed pr/needs-rebase Needs rebase or conflict resolution. labels Sep 4, 2026

@adaamko adaamko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with xunzhuo on named recipes, and the fix is probably structural rather than another loop: ExtractRequiredFilesByModel already walks the config by reflection and picks up mapping files from named recipes for free, while addClassifierModelRequiredFiles reads three flat fields. if the root-weight rule hangs off the same walk (any CategoryModel / PIIModel / PromptGuardConfig it visits, with the same mmbert32k + local checks), reachable named recipes are covered without a second list to maintain.

on the carve-out: the comment and test say "LoRA", but what the code skips is the whole candle backend, and a merged non-LoRA directory on candle is exactly as exposed to the half-download case as the mmbert32k one. fine to leave that for later, but name it as the candle backend so nobody reads the LoRA test as covering it.

conflicts with #3533 and #2828 in the same function, see my note on #2828 for an order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr/needs-author Waiting for author changes or response. wg/router-models-inference-runtime Owned by the Router Models and Inference Runtime Workgroup.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Restore incomplete ModernBERT runtime weights during model healing

10 participants